Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fix hard crash on Android in Prototype tests in release #321

Closed
wants to merge 3 commits into from

Conversation

mrousavy
Copy link
Owner

@mrousavy mrousavy commented Nov 12, 2024

I honestly don't know why this crashes.

But now we check for hasNativeState() also in release builds, not only debug. This will then throw if the function is called on an unbound object (no this/NativeState).

I can only explain this being called by console.log'ing an unbound object (so just the Prototype) - which has an overridden toString function but that requires it to be bound.
I guess if console.log calls toString() and toString() throws, it will catch the error internally.. idk man.

no real production app will call toString() on a prototype in release, so I'm not sure if this is the right approach.

Copy link

vercel bot commented Nov 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
nitro-docs ⬜️ Skipped (Inspect) Nov 12, 2024 10:22am

@mrousavy mrousavy marked this pull request as ready for review November 12, 2024 10:22
@mrousavy
Copy link
Owner Author

mrousavy commented Nov 12, 2024

This code here works:

Screenshot 2024-11-12 at 11 43 44

and this code here crashes:

Screenshot 2024-11-12 at 11 44 11

..same as if I would just not call hasNativeState at all, this also crashes:

Screenshot 2024-11-12 at 11 44 22

So what makes it work? The assert(..) can't be hit, can it? It would crash the app...

@mrousavy
Copy link
Owner Author

Okay some more insights:

  1. This code here crashes:
    auto nativeState = object.getNativeState(runtime);
  2. This code here also crashes:
    object.hasNativeState(runtime);
    auto nativeState = object.getNativeState(runtime);
  3. This code here WORKS, but it doesn't assert/crash the app???:
    if (!object.hasNativeState(runtime)) {
      assert(false);
    }
    auto nativeState = object.getNativeState(runtime);
  4. This code here WORKS:
    if (!object.hasNativeState(runtime)) {
       value.toString(runtime);
    }
    auto nativeState = object.getNativeState(runtime);

So maybe the object is somehow flattened or lazy evaluated in a sense that NativeState isn't properly available?

I am still kinda baffled that the assert does not crash the app, maybe object.hasNativeState(...) is not the same as the internal assert of object.getNativeState(...)?

cc @hannojg @tmikov

@mrousavy
Copy link
Owner Author

Created an issue in the Hermes repo for now: facebook/hermes#1564

@mrousavy
Copy link
Owner Author

I think this can be fixed by just adjusting the code in the example app. No need to slow down all function calls for users by leaving this check in release builds

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant